Walkthrough

Business Hub spawn paths now hide detached Windows consoles and support injected spawning for tests. Slack notification formatting now emits standard section field blocks, while converters accept both section and legacy field payloads.

Changes

Business Hub console launch

Slack field payload handling

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• richard-devbot/SDLC-rstack#474: Updates Slack Block Kit field formatting and parsing across the same notification converters.

Suggested reviewers: richardsongunde

Poem

A rabbit hops where consoles gleam,
Hidden windows guard the stream.
Slack fields bloom in sections bright,
Old shapes still parse just right.
“Binky!” cheers the Hub tonight.

• X
• Mastodon
• Reddit
• LinkedIn

• Hide Win32 console flash when launching Business Hub server via detached node.exe spawn
• Make hub auto-launch spawn injectable and add a regression test for windowsHide
• Fix Slack payload blocks (fields → section) and update Teams/Discord/Text converters

graph TD
  H["windows-console-flash-470.test.js"] --> A["auto-launch.js"] --> B["spawn(node.exe) windowsHide"] --> C["Business Hub server"]
  H --> D["rstack-sdlc.ts"] --> B
  E["notifications/index.js"] --> F["Slack blocks: section.fields"] --> G["converters: teams/discord/text"]

Loading

The following are alternative approaches to this PR:

1. Centralize background process spawning

• ➕ Prevents missing windowsHide at future detached spawn call sites
• ➕ Allows consistent defaults (stdio/env/detached) and easier testing
• ➖ Requires refactoring multiple files and potentially wider blast radius
• ➖ May be hard to standardize if spawn semantics differ across features

2. Use a GUI-subsystem launcher (nodew.exe) on Windows

• ➕ Avoids console windows even without windowsHide
• ➕ Could simplify Windows-specific process behavior
• ➖ Adds platform-specific branching and distribution complexity
• ➖ May change debugging/stdio expectations

3. Validate Slack payloads against a Block Kit schema/library

• ➕ Catches invalid block shapes (like type: 'fields') before sending
• ➕ Improves long-term safety as payloads evolve
• ➖ Adds dependency/runtime overhead
• ➖ May require migration work across notification call sites

Recommendation: The PR’s approach is the right minimal, targeted fix: apply windowsHide to the missed detached node.exe spawns and add a regression test, while correcting Slack Block Kit block types and keeping converters backward-compatible. Consider a follow-up to centralize detached/background spawns to avoid repeating platform-specific flags across the codebase.

Bug fix (6) +25 / -6

auto-launch.jsInjectable spawn + windowsHide on hub server spawn +10/-1

Injectable spawn + windowsHide on hub server spawn

• Adds an injectable spawn implementation (spawnImpl) to make hub-launch behavior testable. Updates the detached node.exe spawn to include windowsHide: true to prevent Win32 console flashing.

src/hooks/auto-launch.js

rstack-sdlc.tsHide console flash when Pi launches hub server +5/-0

Hide console flash when Pi launches hub server

• Updates the detached node.exe spawn used to launch the Business Hub server to include windowsHide: true, preventing a console window flash on Windows.

src/integrations/pi/rstack-sdlc.ts

discord.jsRead Slack fields from section blocks (tolerate legacy type) +2/-1

Read Slack fields from section blocks (tolerate legacy type)

• Adjusts Slack-to-Discord conversion to treat fields as belonging to section blocks while still accepting legacy blocks with type: 'fields'. Prevents dropped fields after Slack payload shape correction.

src/notifications/channels/discord.js

teams.jsExtract facts from section.fields (tolerate legacy type) +2/-1

Extract facts from section.fields (tolerate legacy type)

• Updates Teams conversion to read fields from section blocks (the valid Block Kit shape) while tolerating legacy type: 'fields' payloads for backward compatibility.

src/notifications/channels/teams.js

text.jsFlatten section.fields blocks to text (tolerate legacy type) +2/-1

Flatten section.fields blocks to text (tolerate legacy type)

• Updates the plain-text converter to treat fields as part of section blocks, while still accepting legacy type: 'fields' blocks. Maintains readable text output for notifications.

src/notifications/channels/text.js

index.jsFix Slack Block Kit blocks: 'fields' → 'section' +4/-2

Fix Slack Block Kit blocks: 'fields' → 'section'

• Corrects Slack payload builders to emit fields on a section block (Block Kit-compatible) rather than an invalid 'fields' block type. This prevents Slack webhook rejections due to invalid attachments.

src/notifications/index.js

Tests (1) +29 / -1

windows-console-flash-470.test.jsRegression test for windowsHide on hub server spawns +29/-1

Regression test for windowsHide on hub server spawns

• Expands the existing #470 console-flash coverage to include the detached node.exe server-launch spawn. Uses the new spawnImpl injection to assert windowsHide: true without creating a real background process.

tests/windows-console-flash-470.test.js

Description

The PR repeats the same Slack Block Kit fields-handling logic (and explanatory comment) across
multiple notification channel converters, creating a DRY violation and increasing maintenance risk
if the parsing behavior changes again.

Code

src/notifications/channels/discord.js[R24-28]

+      // Fields ride on a section block; 'fields' is tolerated for older payloads.
+      if ((block.type === 'section' || block.type === 'fields') && block.fields) {
        for (const f of block.fields) {
          const raw = f.text || '';
          const parts = raw.split('\n');

Relevance

⭐⭐ Medium

No evidence team enforces DRY in converters; multi-channel code merged as-is (PR #39).

PR-#39

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1399536 requires eliminating duplicated logic. The same `block.type === 'section'
|| block.type === 'fields'` conditional and field-iteration logic was added in three separate
channel converters, indicating copy/paste duplication rather than a shared helper.

src/notifications/channels/discord.js[24-33]
src/notifications/channels/teams.js[27-40]
src/notifications/channels/text.js[26-34]
Skill: plan-eng-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Slack `fields` parsing logic is duplicated across multiple notification channel converters, violating DRY and making future schema tweaks easy to miss in one of the implementations.

## Issue Context
The same conditional logic and loop appear in `discord.js`, `teams.js`, and `text.js` to support both the correct Block Kit shape (`section` with `fields`) and a legacy tolerated shape (`type: 'fields'`).

## Fix Focus Areas
- src/notifications/channels/discord.js[24-33]
- src/notifications/channels/teams.js[27-40]
- src/notifications/channels/text.js[26-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The test hardcodes the port value 58471 instead of using a named constant. This reduces
readability and makes future updates more error-prone.

Code

tests/windows-console-flash-470.test.js[R109-112]

+  const originalPort = process.env.RSTACK_BUSINESS_PORT;
+  const originalNoBrowser = process.env.RSTACK_NO_BROWSER;
+  process.env.RSTACK_BUSINESS_PORT = '58471';
+  process.env.RSTACK_NO_BROWSER = '1';

Relevance

⭐⭐ Medium

No prior accepted/rejected reviews about replacing magic numbers with named constants in tests.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400588 requires replacing magic numbers with named constants; the test sets
process.env.RSTACK_BUSINESS_PORT to a literal '58471' without defining a descriptive constant.

tests/windows-console-flash-470.test.js[109-112]
Skill: code-patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A literal port number is used directly in the test (`'58471'`), which violates the magic-number rule.

## Issue Context
This port is intended to be an effectively-unused port to force `isPortOpen()` to return false.

## Fix Focus Areas
- tests/windows-console-flash-470.test.js[109-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The new autoLaunchBusinessHub regression test assumes port 58471 is unused so isPortOpen()
returns false; if any process is listening on 127.0.0.1:58471, autoLaunchBusinessHub() will
early-return and never call the injected spawnImpl, causing a nondeterministic test failure.

Code

tests/windows-console-flash-470.test.js[R107-123]

+  // A high, effectively-never-bound port so isPortOpen() resolves false and
+  // the function takes the "spawn a fresh instance" branch under test.
+  const originalPort = process.env.RSTACK_BUSINESS_PORT;
+  const originalNoBrowser = process.env.RSTACK_NO_BROWSER;
+  process.env.RSTACK_BUSINESS_PORT = '58471';
+  process.env.RSTACK_NO_BROWSER = '1';
+  try {
+    await autoLaunchBusinessHub('/tmp/project', { spawnImpl: recordingSpawn(calls) });
+  } finally {
+    if (originalPort === undefined) delete process.env.RSTACK_BUSINESS_PORT; else process.env.RSTACK_BUSINESS_PORT = originalPort;
+    if (originalNoBrowser === undefined) delete process.env.RSTACK_NO_BROWSER; else process.env.RSTACK_NO_BROWSER = originalNoBrowser;
+  }
+  assert.equal(calls.length, 1);
+  assert.equal(calls[0].cmd, process.execPath);
+  assert.equal(calls[0].options.detached, true);
+  assert.equal(calls[0].options.windowsHide, true);
+});

Relevance

⭐⭐ Medium

Flakiness-hardening sometimes rejected (e.g., tmpdir isolation rejected in PR #258); no clear
port-flake precedent.

PR-#258

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test sets a fixed port and expects exactly one spawnImpl call, but the production
implementation checks whether the port is already accepting TCP connections and returns early if it
is, which bypasses the spawn path entirely.

tests/windows-console-flash-470.test.js[105-123]
src/hooks/auto-launch.js[10-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`tests/windows-console-flash-470.test.js` hard-codes `RSTACK_BUSINESS_PORT=58471` to force `autoLaunchBusinessHub()` down the spawn path. Because `autoLaunchBusinessHub()` performs a real TCP probe (`isPortOpen`) before spawning, a listener on that port will flip the branch and make the test fail intermittently.

### Issue Context
This test is intended to validate `windowsHide: true` on the Business Hub *server-launch* spawn. It should be deterministic and not depend on the machine’s ambient port usage.

### Fix Focus Areas
- src/hooks/auto-launch.js[10-35]
- tests/windows-console-flash-470.test.js[105-123]

### Suggested fix
Add a second injectable dependency to `autoLaunchBusinessHub`, e.g. `opts.isPortOpenImpl ?? isPortOpen`, and use that instead of calling `isPortOpen` directly. In the test, pass `isPortOpenImpl: async () => false` so the spawn path is guaranteed without relying on any specific port.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The updated regression test comment block references #470/#471 but does not include the required
found-date and QA report path metadata. This reduces traceability for why/when the regression
coverage was added and where the original report lives.

Code

tests/windows-console-flash-470.test.js[R9-16]

+ * The original #470/#471 fix covered the browser-open spawn (cmd.exe /c
+ * start) at each call site but missed the OTHER spawn each of those files
+ * makes: launching the Business Hub SERVER itself by spawning node.exe
+ * directly with detached: true. node.exe is a console-subsystem executable,
+ * so that spawn flashes a console too, independent of the shell:true case —
+ * it kept flashing after #471 merged. Covered below for both the Claude
+ * Code hook (auto-launch.js) and the Pi integration (rstack-sdlc.ts).
+ *

Relevance

⭐⭐ Medium

No historical evidence for required regression-test attribution metadata (found-date/QA report
path).

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400067 requires regression tests to include attribution metadata including the
issue ID, what broke, the date found, and a QA report reference. The updated header comment includes
the issue IDs and description but still lacks a found date and QA report path.

tests/windows-console-flash-470.test.js[1-18]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The regression test header comment is missing required attribution metadata (date found and QA report path).

## Issue Context
Compliance requires regression tests to include: issue ID, what broke, date found, and QA report path.

## Fix Focus Areas
- tests/windows-console-flash-470.test.js[1-18]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

A new regression test was added by modifying an existing test file instead of creating a new
dedicated regression test file. This violates the requirement that regression tests be added only as
new files to avoid coupling to existing suites.

Code

tests/windows-console-flash-470.test.js[R105-123]

+test('auto-launch.js autoLaunchBusinessHub: windowsHide:true on the server-launch spawn (missed by #471)', async () => {
+  const calls = [];
+  // A high, effectively-never-bound port so isPortOpen() resolves false and
+  // the function takes the "spawn a fresh instance" branch under test.
+  const originalPort = process.env.RSTACK_BUSINESS_PORT;
+  const originalNoBrowser = process.env.RSTACK_NO_BROWSER;
+  process.env.RSTACK_BUSINESS_PORT = '58471';
+  process.env.RSTACK_NO_BROWSER = '1';
+  try {
+    await autoLaunchBusinessHub('/tmp/project', { spawnImpl: recordingSpawn(calls) });
+  } finally {
+    if (originalPort === undefined) delete process.env.RSTACK_BUSINESS_PORT; else process.env.RSTACK_BUSINESS_PORT = originalPort;
+    if (originalNoBrowser === undefined) delete process.env.RSTACK_NO_BROWSER; else process.env.RSTACK_NO_BROWSER = originalNoBrowser;
+  }
+  assert.equal(calls.length, 1);
+  assert.equal(calls[0].cmd, process.execPath);
+  assert.equal(calls[0].options.detached, true);
+  assert.equal(calls[0].options.windowsHide, true);
+});

Relevance

⭐ Low

Repo commonly extends existing test files; new regression tests added in existing suite (PR #167).

PR-#167

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1400748 requires that adding regression tests be done via new test files only, but
the PR adds a new regression test block inside the existing
tests/windows-console-flash-470.test.js file.

tests/windows-console-flash-470.test.js[105-123]
Skill: qa-testing

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A regression test was added by editing an existing test file, but regression tests must be added as new test files only.

## Issue Context
The PR adds a new regression test for the hub server spawn console-flash case (missed by #471) directly into `tests/windows-console-flash-470.test.js`.

## Fix Focus Areas
- tests/windows-console-flash-470.test.js[105-123]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools